Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parry benchmarks compilation #255

Merged
merged 6 commits into from
Sep 16, 2024

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Sep 2, 2024

Fixes compiling for parry benchmarks

There has been a few API changes since their creation, especially around ConvexHull, and in a lesser amount Capsule.

@@ -143,7 +142,7 @@ bench_method!(
bounding_sphere: BoundingSphere,
c: ConvexHull,
m: Isometry3<f32>
);
);*/
Copy link
Contributor Author

@Vrixyz Vrixyz Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConvexHull is no longer a type as iitself, but rather an alias to Vec<Point<Real>>, I think it would have value to be its own type, but slightly out of scope for this PR.

@sebcrozet I'd appreciate instructions as how to proceed:

Should I straight up remove that code, or keep it in and create a tracking issue (+ comment) ?

Then, should I either:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ConvexHull shpe type has been renamed ConvexPolyhedron in 3D and ConvexPolygon in 2D.

crates/parry3d/benches/all.rs Outdated Show resolved Hide resolved
@Vrixyz Vrixyz requested a review from sebcrozet September 2, 2024 13:53
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! Note that the ConvexHull shpe type has been renamed ConvexPolyhedron in 3D and ConvexPolygon in 2D.

crates/parry3d/Cargo.toml Outdated Show resolved Hide resolved
crates/parry3d/benches/all.rs Outdated Show resolved Hide resolved
Comment on lines 15 to 19

#[cfg(feature = "dim2")]
type ConvexHull = Vec<Point<Real>>;
#[cfg(feature = "dim3")]
type ConvexHull = (Vec<Point<Real>>, Vec<[u32; 3]>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg(feature = "dim2")]
type ConvexHull = Vec<Point<Real>>;
#[cfg(feature = "dim3")]
type ConvexHull = (Vec<Point<Real>>, Vec<[u32; 3]>);

@@ -143,7 +142,7 @@ bench_method!(
bounding_sphere: BoundingSphere,
c: ConvexHull,
m: Isometry3<f32>
);
);*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ConvexHull shpe type has been renamed ConvexPolyhedron in 3D and ConvexPolygon in 2D.

@@ -72,7 +72,7 @@ macro_rules! bench_method_gen (

unsafe {
let val: $tres = test::black_box($arg.get_unchecked(i).$method($(unref($args.get_unchecked(i)),)*));
drop(val);
let _ = val;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was that change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using drop results in a warning:

warning: calls to `std::mem::drop` with a value that implements `Copy` does nothing
   --> crates/parry3d/benches/bounding_volume/../common/macros.rs:75:21

Aabb and BoundingSphere are concerned. I don´t think this is an opportunity to make benchmarks more accurate (if we're unexpectedly copying).

Actually I'm not exactly sure why this drop is necessary. As we're in an unsafe call, I guess it might be useful, but not really used in current usage. It doesn´t hurt being here I guess.

@Vrixyz Vrixyz requested a review from sebcrozet September 9, 2024 08:56
@Vrixyz Vrixyz merged commit 81229a5 into dimforge:master Sep 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants